C# client: fix default values for DateTime etc, numeric and boolean defaults were not applied#7414
C# client: fix default values for DateTime etc, numeric and boolean defaults were not applied#7414WolfgangHG wants to merge 3 commits intomicrosoft:mainfrom
Conversation
baywet
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Added a comment to help drive this to completion.
In addition, could you please add unit tests for the code method writer?
@baywet The code that I changed is completely covered by Could you give me a hint what kind of testing you expect and where I could copy code? |
|
This is essentially an integration test. Thank you for initially adding it. Did you come across the default value tests in coemethodwritertests? I'm suggesting to add more there. Let me know if you have additional questions |
29dc124 to
e073db1
Compare
Perfect, that's what I actually was looking for but didn't find ;-). I added the test for "float value needs the suffix f" to Is the integration test (which parses the full json file) still required? I think so, because it brings the full demo file for this problem. |
|
Thanks for adding the unit tests! I think we're getting really close at this stage. |
|
I added another small commit. Some integration tests (including my new one) did not set I also experimented a bit (not committed) and added Roslyn compilation to the generated clients of the "Kiota.Builder.IntegrationTests" project. Later I found that there is a big integration tests suite in the "it" directory that is only run during Github pull request verification. Probably, there compilation is also done. But this big test probably does not cover the default value handling of this pull request. What do you think? Is the approach to compile the "Kiota.Builder.IntegrationTests" clients worth the effort? Or is this unnecessary? |
Not worth the time. Both in terms of setup, and then in terms of additional runtime of the tests. This is why we set this up as integration tests run by the CI, and not integration tests run as the local test suite. But thank you for considering it. |
|
@baywet I see that a lot of tests are failing, most in java clients. Is this caused by my code? Maybe because I detect default values for basic data types now and they are not handled properly when generating the java client? E.g. here: |
|
I hope I fixed the java problems - a client created from my sample JSON file works again, and the client from The sample model "WeatherForecast" model looks like this: public WeatherForecast() {
this.setBoolValue(true);
this.setDateOnlyValue(LocalDate.parse("1900-01-01"));
this.setDateValue(OffsetDateTime.parse("1900-01-01T15:00:00+00:00"));
this.setDateValueLocalTime(java.time.LocalDateTime.parse("1900-01-01T00:00:00").atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime());
this.setDecimalValue(25.5d);
this.setDoubleValue(25.5d);
this.setFloatValue(25.5f);
this.setGuidValue(UUID.fromString("00000000-0000-0000-0000-000000000000"));
this.setSummary("Test");
this.setTemperatureC(15);
this.setTimeValue(LocalTime.parse("00:00:00"));
}Specials compared to C#:
Update 2026-03-27: after thinking once more about the "datetime without timezone" problem, I understood that ISO8601 allows also datetime values without timezone - thats "local time (unqualified)" (https://en.wikipedia.org/wiki/ISO_8601). OffsetDateTime cannot parse them, it fails. Now to the other problems - as I don't know Go or Dart, this might be harder... |
|
About GO: Currently, it generates this: m.SetBoolValue(true)
m.SetDecimalValue(25.5)
m.SetDoubleValue(25.5)
m.SetFloatValue(25.5)...which results in errors like Following https://www.codegenes.net/blog/how-to-set-bool-pointer-to-true-in-struct-literal/, this could be done like this: m.SetBoolValue(func() *bool {
b := true
return &b
}())
m.SetDecimalValue(func() *float64 {
b := 25.5
return &b
}())
m.SetFloatValue(func() *float32 {
b := float32(25.5)
return &b
}())The float value literal causes a compilation error, and the float64 value would do the same if the default has no decimal point. Is there any suffix/prefix to define a float32/float64 literal, or must it be done with @baywet What do you think about my suggestion? By the way: due to my lack of knowledge of theses languages, I would prefer to ignore the date/time default handling hat I added for C#/Java, as they probably never were an issue with GO or Dart previously ;-). |
|
Also a PHP error: https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135791?pr=7414#step:17:139 The DART error: https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135767?pr=7414#step:17:75 |
|
Thanks for looking into this! For dart maybe we can ask help from @ricardoboss on this. For go, this is verbose, but I don't have a better solution unless we add short hands in the abstractions library. As for which types are supported, I believe we should strive for parity between stable languages here. |
|
Dart requires default values to be compile time constant, so you can't actually have a default parameter like |
|
Making some slow progress with Go and requesting feedback on the current result. This is the generated Go code for my "WeatherForecast" sample class. I verified that it works and all fields are assigned the expected values. func NewWeatherForecast()(*WeatherForecast) {
m := &WeatherForecast{
}
boolValueValue := true
m.SetBoolValue(&boolValueValue)
dateOnlyValueValue, _ := i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseDateOnly("1900-01-01")
m.SetDateOnlyValue(dateOnlyValueValue)
dateValueValue, _ := i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Parse(i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.RFC3339, "1900-01-01T00:00:00+00:00")
m.SetDateValue(&dateValueValue)
decimalValueValue := float64(25.5)
m.SetDecimalValue(&decimalValueValue)
doubleValueValue := float64(25.5)
m.SetDoubleValue(&doubleValueValue)
floatValueValue := float32(25.5)
m.SetFloatValue(&floatValueValue)
guidValueValue, _ := i561e97a8befe7661a44c8f54600992b4207a3a0cf6770e5559949bc276de2e22.Parse("00000000-0000-0000-0000-000000000000")
m.SetGuidValue(&guidValueValue)
summaryValue := "Test"
m.SetSummary(&summaryValue)
temperatureCValue := int32(15)
m.SetTemperatureC(&temperatureCValue)
timeValueValue, _ := i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseTimeOnly("00:00:00")
m.SetTimeValue(timeValueValue)
return m
}Update 2026-03-31: for DateTime values, I now check the default value whether it contains a timezone or not. A value without timezone results fails to parse in RFC3339 format. But it works to do this (this automatically applies the local timezone to the DateTime): dateValueLocalTimeValue, _ := i33...d7e.ParseInLocation("2006-01-02T15:04:05", "1900-01-01T00:00:00", i33...d7e.Now().Location())
m.SetDateValueLocalTime(&dateValueLocalTimeValue)The first argument ( Attached is the full file: Question 1: is there anything reusable? I see that the generated |
|
Thank you for the additional information. Yes, I like this approach! It's verbose, but consistent at least. Q1: I don't think you'll be able to easily reuse the parse node infrastructure because that not only requires you to have access to the parse node factories, but also get bytes from string, and the representation might vary based on the mime type/available implementation. All of which add layers of complexity that calling a static factory is much simpler. Maybe it'd be worth refactoring things to make sure the parts that emit those lines are defined once and not duplicated. Q2: I've always found that part to be odd in Go's design. Essentially "constructors" don't typically return an error, but the way errors work, they require an extra return value. It does not fit well. Since we don't want to introduce breaking changes, this effectively leaves us with two choices:
My opinion is that failing to set a default because the spec is odd, or because the static factory fails to parse a value, is not a huge deal which should lead the application to crash. So panicking is probably too aggressive. Of course that's something that can always be updated later on if people have issues with errors being ignored. Let me know if you have any additional comments or questions. |
|
Here we go with the Go client. Hope I didn't break it - not having seen Go before three days ago... @baywet Thanks for the feedback about error handling. I agree completely - a parse error while applying the defaults should be avoided. If a user runs into a parse problem, she could still add error logging in the generated code. I am very confused that the Go test @baywet Could you start the Github CI test run so that I can see whether my changes fix the failing Java and Go tests? PHP and Dart are still on my TODO list, but not before next Tuesday. I also don't know those two languages ;-) |
2564a1b to
c96ed51
Compare
|
Are those GO errors also caused by me? For example https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047630?pr=7414#step:17:35:
|
I'd say yes since one of the files it's erroring with is local (not pulled from an external repo) and since this is only happening on this branch. Let us know if you have any additional comments or questions. |
|
Yes, it was me ;-). Previously, a default value was always set as With the latest commit, I brought back the old behavior: m.SetAdditionalData(make(map[string]any)) |
|
@baywet There is one failing Java test: https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047215?pr=7414#step:17:201 The error is: The reason is this snippet: "make_latest": {
"default": true,
"description": "Specifies whether this release should be set as the latest release for the repository. ....",
"enum": [
"true",
"false",
"legacy"
],
Shouldn't the default for the enum value be placed in quotes here? Before, it was not detected. Kiota now creates this java snippet: public WithReleasePatchRequestBody() {
this.setAdditionalData(new HashMap<>());
this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.forValue(true));
}
I could work around by adding quotes to enum default values if they are missing: if (propWithDefault.Type is CodeType propertyType && propertyType.TypeDefinition is CodeEnum enumDefinition)
{
if (!defaultValue.StartsWith('"'))
{
defaultValue = $"\"{defaultValue}\"";
}
defaultValue = $"{enumDefinition.Name}.forValue({defaultValue})";
}How to continue? Add the workaround? Or if my understanding is correct and the syntax is invalid, would it be worth a bug report? Could you also trigger another run of the tests to verify that the Go problems are fixed? |
|
In this example, the default value (in the OpenAPI description) is wrong IMHO. That's because the enum value is "true" (a string) but the default is I'd be ok suppressing the specific operation that's bringing this type in the configuration here for that reason. |
a159a9e to
ac2be7d
Compare
|
I created github/rest-api-description#6100. Seems they fixed one of the problems already. But I am a bit irritated because the file that Kiota downloads differs a lot from the github file. Did I look at the proper file ;-)? @baywet Why not use the approach of the C# client generator instead of calling a parsing method? kiota/src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs Lines 256 to 259 in e19028c Currently generated Java code: public WithReleasePatchRequestBody() {
this.setAdditionalData(new HashMap<>());
this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.forValue("true"));
}The C# approach would result in this code: public WithReleasePatchRequestBody() {
this.setAdditionalData(new HashMap<>());
this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.True);
}Here, the generated code compiles without errors. Could we use this code? By the way: the "config.json" that you linked contain some suppressions for |
Yes, most descriptions come from the APIs.guru index. Which is often out of date sadly. This is a bit outside of the scope of this pull request but you could do a could of things:
In any case, if any changes are required in kiota's repo, please open a separate pull request to avoid mixing concerns/spead up the review process.
I'm not sure/can't remember why we'd have a difference in the approach between languages here. Could be as simple as the initial language implementers not coordinating. Happy to see things getting aligned assuming it doesn't introduce regressions.
yes, it's probably an oversight. Please do that in a separate PR though. Thanks for the continuous work here. Let me know if you have any additional comments or questions. |
|
About a ruby mockserver test: I did not even touch the ruby part of Kiota, as there were no errors: the generated code just assigned the string defaults, and there are no datatype definitions for properties. But there is probably a difference beetween and So, I will try to parse the default values based on the data types returned by the JsonParseNode |
|
I committed changes to Ruby and added a MockServer test (that did not exist before). Before, it generated this code: def initialize()
@bool_value = true
@date_only_value = "1900-01-01"
@date_value = "1900-01-01T00:00:00+00:00"
@date_value_local_time = "1900-01-01T00:00:00"
@decimal_value = 25.5
...
@enum_value = "one"
...
@guid_value = "00000000-0000-0000-0000-000000000000"
...
@time_value = "00:00:00"
endNow it looks like this: def initialize()
@bool_value = true
@date_only_value = Date.parse("1900-01-01")
@date_value = DateTime.parse("1900-01-01T00:00:00+00:00")
@date_value_local_time = DateTime.parse("1900-01-01T00:00:00")
@decimal_value = 25.5
...
@enum_value = Integration_test::Client::Models::WeatherForecastEnumValue[:One]
...
@guid_value = UUIDTools::UUID.parse("00000000-0000-0000-0000-000000000000")
...
@time_value = Time.parse("00:00:00")
endParsing of DateTime defaults without timezone ("local time") actually does not work as expected - it creates an UTC value. @date_value_local_time = Time.local(2026, 6, 1, 13, 0, 5, 0).to_datetimeBut the method Note that I also changed the handling of the enum value, which assigned a string before. Looking at the data that was deserialized from the Mockserver Json reponse, I saw that the Json data was also parsed as a real enum value, so I tried to change the code the same way (copied from other writers ;-)) ===================================== All other Mockserver tests have a directory "basic" that I treated as a template for new tests. Here, the service invocation for "InheritingErrors.yaml" resulted in a compilation error: It is caused by this line in "kiota\it\ruby\lib\integration_test\client\api\v1\topics\topics_request_builder.rb": return @request_adapter.send_async(request_info, Binary, error_mapping)So the "basic" test file has a comment line for the service invocation. I wonder why this action before did not report errors (latest run: https://github.com/microsoft/kiota/actions/runs/24415122058/job/71353398995). Does ruby not compile the full generated code? |
|
This pull request is again ready for review and for merging. I worked around microsoft/kiota-abstractions-ruby#82 by creating a custom |
baywet
left a comment
There was a problem hiding this comment.
Thank you for the great continuous work on this. Left a comment here. I'll also ask copilot to review. I've designed a skill for the sanitation, hopefully it catches things I've missed. We also have failing CI to look into. To give you an idea of progress, I've reviewed 54 out of 55 changed files so far.
|
I fixed all comments besides one - see my comment on your feedback. CI should also work again. It was a dumb formatting issue - the last change before committing - it sounded so trivial that no further test run seemed to be required ;-) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //OffsetDateTime can only parse a date string that contains a timezone or "Z" (format "DateTimeFormatter.ISO_OFFSET_DATE_TIME"). | ||
| "offsetdatetime" when defaultValue.Contains('+', StringComparison.InvariantCulture) || defaultValue.Contains('Z', StringComparison.OrdinalIgnoreCase) => $"OffsetDateTime.parse({defaultValue})", | ||
| //Otherwise: consider it a local date/time value. Create a OffsetDateDateTime in the current timezone. | ||
| "offsetdatetime" => $"java.time.LocalDateTime.parse({defaultValue}).atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime()", | ||
| "localtime" => $"LocalTime.parse({defaultValue})", |
There was a problem hiding this comment.
Timezone detection for OffsetDateTime defaults only checks for '+' or 'Z'. RFC3339 offsets can also be negative (e.g. ...-05:00), which would incorrectly fall into the LocalDateTime.parse() branch and throw at runtime. Consider detecting an offset more robustly (e.g. regex /[+-]\d{2}:\d{2}$/ or DateTimeFormatter parsing attempt) so negative offsets are handled by OffsetDateTime.parse().
There was a problem hiding this comment.
Copilot is correct. I extracted the code to a helper method StringExtensions.IsDateTimeWithOffset, where I detect whether the string ends with "Z" or matches the regex suggested by Copilot.
There is also a test for this method.
@baywet Could you review this? Is there a better place for such a method?
There was a problem hiding this comment.
Kiota's general direction on those kind of issues has been to avoid patching for bad descriptions. Because it's a rabbit hole with infinite possibilities.
If we get a offsetdatetime type at this place it's because the description type was string and format was date. To enable interoperability, the registry suggests this should comply with RFC3339 date-time, which is mandated to contain an offset (Z or numerical).
If the corresponding default value matches this, then the equivalent Java (or any language) API should be able to parse it without any issues.
If the corresponding default value DOES NOT match this, then the description is at worst not interoperable, at best inconsistent with itself.
In both cases, this is not something that kiota should cater to, there are too many possibilities with such issues, it'd make things unmaintainable.
My suggestion is to drop the parsing method you added, drop the when qualifier on the first offsetdatetime case, and then drop the second offsetdatetime case entirely.
If an integration test is failing because of that, we can suppress it as you already know the mechanism for that.
Let me know if you have any additional comments or questions.
|
I fixed the failing CI test (ai detected it also ;-)), and I will think about the rest and continue tomorrow. Detecting wether a datetime default value has a timezone or not might be more difficult than I thought. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The hostile payload's inner quotes must be escaped so it stays inside the string literal | ||
| Assert.Contains(@"DateTimeOffset.Parse(""1900-01-01\""); Evil.Run(\""x"").Date)", result); | ||
| // String default must have newline and quotes escaped | ||
| Assert.Contains(@"""line1\""", result); | ||
| Assert.Contains(@"\n", result); |
There was a problem hiding this comment.
The hostile-payload assertion appears to expect the injected \"); Evil.Run(...) sequence to break out of the string literal (note the \""/double-quote sequence). With SanitizeQuotedStringLiteral escaping inner quotes, the generated code should keep Evil.Run inside the string argument; this assertion is likely incorrect/brittle and may fail (or pass while still allowing injection). Consider asserting that the entire payload remains within a single string literal (e.g., Evil.Run only appears inside the parsed string argument) rather than matching a hard-coded escaped substring.
There was a problem hiding this comment.
@baywet the WritesConstructorWithDefaultValuesSanitizesHostilePayloads was added by you in this commit.
Could you comment on the Copilot comment?
I think the expected string DateTimeOffset.Parse("1900-01-01\"); Evil.Run(\"x").Date) is perfectly fine.
I planned to squash the commits again, but I will delay it until this is sorted out, just to keep the commit history traceable.
By the way: thanks for taking care of sanitation. This is really a bad coincidence that my pull requests might open a window where you closed a lot of doors ;-)
| #generated api code if integration test is run locally | ||
| src/client/ | ||
|
|
||
| vendor | ||
| composer.lock No newline at end of file | ||
| composer.lock |
There was a problem hiding this comment.
This ignore list covers src/client/, but it/exec-cmd.ps1 copies generated PHP code into src/Client/ (capital C) for MockServer tests. On case-sensitive filesystems (Linux CI), src/Client/ won't be ignored and will show up as untracked changes. Consider ignoring both casings (or aligning the copy destination with the ignored path).
There was a problem hiding this comment.
Probably correct, though I don't know whether development is done on Linux.
I also changed the client path to "src\Client" in "get-additional-arguments.ps1", as it turned out in my "defaultvalues" test that the CI fails to find classes if the namespace from "composer.json" is upper case, but a subdir is lower case (see #7414).
But this problem only occured in unit test files, so it is just code consistency without effect.
There was a problem hiding this comment.
let's assume contributors come for a wide variety of OSes. And windows is Ci anyway, to matching the case we expect from our own scripts is the correct path forward here.
eb8851b to
a6bd7a2
Compare
baywet
left a comment
There was a problem hiding this comment.
Thanks for the continuous work here. And sorry about the delay in review lately. Having some health challenges on my end.
| public static bool IsDateTimeWithOffset(this string dateTime) | ||
| { | ||
| if (string.IsNullOrEmpty(dateTime)) return false; | ||
| return dateTime.EndsWith('Z') || dateTime.EndsWith('z') || Regex.IsMatch(dateTime, "[+-]\\d{2}:\\d{2}$"); |
There was a problem hiding this comment.
let's revert that change, we shouldn't get in the business of validating default values.
| //OffsetDateTime can only parse a date string that contains a timezone or "Z" (format "DateTimeFormatter.ISO_OFFSET_DATE_TIME"). | ||
| "offsetdatetime" when defaultValue.Contains('+', StringComparison.InvariantCulture) || defaultValue.Contains('Z', StringComparison.OrdinalIgnoreCase) => $"OffsetDateTime.parse({defaultValue})", | ||
| //Otherwise: consider it a local date/time value. Create a OffsetDateDateTime in the current timezone. | ||
| "offsetdatetime" => $"java.time.LocalDateTime.parse({defaultValue}).atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime()", | ||
| "localtime" => $"LocalTime.parse({defaultValue})", |
There was a problem hiding this comment.
Kiota's general direction on those kind of issues has been to avoid patching for bad descriptions. Because it's a rabbit hole with infinite possibilities.
If we get a offsetdatetime type at this place it's because the description type was string and format was date. To enable interoperability, the registry suggests this should comply with RFC3339 date-time, which is mandated to contain an offset (Z or numerical).
If the corresponding default value matches this, then the equivalent Java (or any language) API should be able to parse it without any issues.
If the corresponding default value DOES NOT match this, then the description is at worst not interoperable, at best inconsistent with itself.
In both cases, this is not something that kiota should cater to, there are too many possibilities with such issues, it'd make things unmaintainable.
My suggestion is to drop the parsing method you added, drop the when qualifier on the first offsetdatetime case, and then drop the second offsetdatetime case entirely.
If an integration test is failing because of that, we can suppress it as you already know the mechanism for that.
Let me know if you have any additional comments or questions.
| #generated api code if integration test is run locally | ||
| src/client/ | ||
|
|
||
| vendor | ||
| composer.lock No newline at end of file | ||
| composer.lock |
There was a problem hiding this comment.
let's assume contributors come for a wide variety of OSes. And windows is Ci anyway, to matching the case we expect from our own scripts is the correct path forward here.
| [Fact] | ||
| public void DetectsDateTimeOffset() | ||
| { | ||
| Assert.True("1900-01-01T00:00:00+00:00".IsDateTimeWithOffset()); |
There was a problem hiding this comment.
this needs to be reverted as well.
There was a problem hiding this comment.
OK, two issues became part of the same conversation. This might get complicated.
First the easier one: my latest change from "client" to "Client" (as suggested by the AI) breaks CI. I will have to fix ".gitignore" and the powershell script. You wrote "And windows is Ci anyway" - did you mean the Github CI? But it runs with Ubuntu.
Second is the (local) datetime issue: The link you provided mentions two date time types:
- date-time-local RFC3339 date-time without the timezone component
- date-time date and time as defined by date-time - RFC3339
"date-time-local" is probably bad design, but it works if you know that your service is only used e.g. by German customers (as happens for our own REST service).
As I didn't have to care for timezones, I used the .NET "DateTime" type for properties in our own service (with "DateTimeKind = Unknown"), and the Swashbuckle layer generated a OpenAPI spec from my controllers and classes, also using datetime strings without timezone.
During the work on this pull request, I asked in the Swashbuckle forum and they gave me a workaround to use a JsonConverter to set a "DateTimeKind = Local", which would result in DateTime values being serialized with the local timezone.
Supporting "date-time-local" might be a bigger problem, as Kiota does not seem to support this type at all.
I found this issue https://www.github.com/OAI/OpenAPI-Specification/issues/4759, and you probably know it as you reviewed the corresponding pull request ;-).
The code in this pull request that checks whether a DateTime value has a timezone or not is mainly a workaround for my own badly designed service ;-). I could fix it to use proper timezone values with the help of a JsonConverter, but I would have to keep it backwards compatible for existing customers that don't want to fix their clients.
Let's continue this tomorrow, it is late now...
There was a problem hiding this comment.
I don't know whether it is allowed by RF3339, but my interpretation was up to now that a datetime without timezone is in local timezone. But maybe I got confused and took this information from ISO8601 where it is allowed.
Some languages (C# and DateTimeOffset) seem to handle this more lenient, others (Java, Go) don't and raise an exception. That's why I added fallback code to parse a datetime string without timezone.
What do you think? Shall I revert the fallback changes and require "datetime with timezone" values always?
This would also mean I have to fix my own service in a version "2" ;-)
There was a problem hiding this comment.
Not sure why we're having both conversations here. By CI I meant case insensitive. Sorry for the confusion
There was a problem hiding this comment.
I was referring to date-time, not the local variant to be clear.
There was a problem hiding this comment.
Let's revert the fallback for now and see if people complain about this edge case, which I suspect they won't.
(Sorry for the fragmented reply, I'm on my phone)
There was a problem hiding this comment.
I hopefully fixed the failing PHP test in CI ("continous integration", not "case insensitive" ;-)).
Reason for the change: when I added a PHP integration test, it failed in the Github CI (linux) because it couldn't find classes in namespace "Client..." - the directory was named "client". This was fixed in "exec-cmd.ps1" which copied the generated client from "client" to a dir "Client" in the integration test folder.
Copilot suggested to change the casing also for the "client" directory where the generated client is written to. This broke CI.
Will revert the "local date time" handling the next few days.
There was a problem hiding this comment.
The code for "datetime without timezone" was reverted. Pull request is again ready for review.
81e356d to
336aaa5
Compare
…ric and boolean defaults were not applied, add mockserver tests
336aaa5 to
ce6fd8b
Compare
This is an attempt to fix #7404
If the OpenAPI spec contains model fields with format
date-time,date,timeoruuidand default values, the generated client did not compile because the string value was assigned directly to the property.Default values for numeric types and boolean were not applied at all, because the code ignored all default values that did not start with a quote.
I also tried to add a test and placed my json file in "Kiota.Builder.IntegrationTests" and added a simple test to "GenerateSample.cs", but only for C#. Other languages apparently handle the default values differently.
This test just runs code generating. It might also make sense to test the generated file. But I don't have an idea how you would do it. It would probably perfect if the client code could be compiled using Roslyn and then the model class could be loaded from the dynamic assembly and the field values could be verified. Or I could take at least a look in the generated class, as e.g. test "GeneratesUritemplateHintsAsync" does. What do you think?